Core: replace the eval in OptionsCreator.py #5828
Conversation
github-actions
bot
added
affects: core
| if self.options[name].isnumeric() or self.options[name] in ("True", "False"): | ||
| self.options[name] = eval(self.options[name]) | ||
| if self.options[name].isnumeric(): | ||
| self.options[name] = int(self.options[name]) |
There was a problem hiding this comment.
I suppose this isn't a new issue, but .isnumeric() doesn't seem like the check that's actually wanted here. For instance, a Range that's set to a negative value and then toggled and untoggled random will end up being output as a string since something like "-1".isnumeric() gives False, although it should still work if from_any is used. If any int-castable value here should be accepted, that should be done by trying to do it in a try:.
I'd also be concerned about other similar issues when trying to determine type based just on string, although something like a choice called True seems to be spared by being lowercase.
There was a problem hiding this comment.
I suppose this isn't a new issue
I'm not intending to change functionality here, I think that can be PR-ed separately.
preference is to use
ast.literal_eval
I don't think ast.literal_eval is suitable. ast.literal_eval is intended for parsing ast nodes and strings containing arbitrary python literals as python code. By using ast.literal_eval, the code would be implying that it is expecting to parse arbitrary nodes or strings because that is the task ast.literal_eval is designed for. The actual task at hand is simpler, so should use simpler, more fitting tools.
beauxq
left a comment
beauxq
left a comment
There was a problem hiding this comment.
I think the isnumeric is a different issue that can be discussed and fixed in a different PR, and should not block this PR.
gerbiljames
left a comment
gerbiljames
left a comment
There was a problem hiding this comment.
Code makes sense to me, agreed on the isNumeric() change being for another PR
duckboycool
added
waiting-on: core-review
github-actions
bot
removed
the
waiting-on: core-review
6 participants
What is this fixing or adding?
Replaces the
evalin OptionsCreator.py with converting strings toint/booldirectly.eval, while probably safe here, can be a security concern, which could have complicated maintenance and/or refactors.How was this tested?
I clicked the "Random?" button on numeric, boolean and other options, with extra logging to log the new value of
self.options[name]each time, and which conditional branch was being hit.